[CIR] Backport TargetAddressSpaceAttr and Support both existing Lang AS and target AS attributes in pointer types and Global Ops
#1986
Conversation
…guage(clang) and target address-space attributes in pointer types
RiverDave
changed the title
TargetAddressSpaceAttr and Support both existing AS and target AS attributes in pointer types and Global Ops
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
RiverDave
changed the title
TargetAddressSpaceAttr and Support both existing AS and target AS attributes in pointer types and Global OpsTargetAddressSpaceAttr and Support both existing Lang AS and target AS attributes in pointer types and Global Ops
RiverDave
requested review from
andykaylor,
bcardosolopes,
lanza and
xlauko
as code owners
|
I like the overall direction, thanks for taking the time to improve the holistic approach. One nit is to perhaps rename (cc @koparasy) |
| : CIR_AttrConstraint<"::cir::ClangAddressSpaceAttr", "clang address space attribute">; | ||
|
|
||
| def CIR_TargetAddressSpaceAttrConstraint | ||
| : CIR_AttrConstraint<"::cir::TargetAddressSpaceAttr", "target address space attribute">; |
There was a problem hiding this comment.
Is there a plan to eventually merge these two? I feel it's weird to have two different attributes for it, I wonder if it would be more clean if there's a base AddressSpaceAttr and these two different ones are just specializations - using CIR_AnyAddressSpaceAttr for this only work for constraints and it would be nice if in general to pass things around in the terms of the generic version. If so, maybe doing it as part of this PR is a good time.
There was a problem hiding this comment.
I'm not entirely sure if specializations for attributes can be done in MLIR (I might need to research!).
What we could do is to add MemorySpaceAttrInterface to PointerType as a paremeter (Similar to how it's done in the ptr dialect:
Target and Clang address spaces. This last thing I intended to do in a different PR, but I agree It's not entirely correct in its current state.
If the above makes sense I'll work on it soon.
I don’t have a strong objection to the implementation itself — the patch is clear and seems consistent with the upstream direction. My only hesitation is around the conceptual cost of introducing two separate address-space attributes. Address spaces are already a tricky area, and users can also influence them via language-level attributes, so reasoning about them tends to get complicated quickly. Do we have a long-term plan for how |
Hey :) thanks for the comment, we initially had laid down support for it to be unified as it is currently down here in the incubator. The decision to split both attributes came from feedback from one of the mantainers of the core mlir dialects. llvm/llvm-project#161028 (comment) (I'd suggest thoroughly looking at the discussion for full context). Our ultimate goal is to align with other upstream dialects, specifically the ptr dialect and how it attaches memory spaces to it. I think It is fair for both of them to remain distinct in CIR at least. They would converge on the lowering stage as LLVM ir can only represent numeric address spaces. However, I think @xlauko and @andykaylor have more experience designing dialects and at the end — might have a better answer for you. |
Bringing the changes we performed at: llvm/llvm-project#161028 down to the incubator isn't as straight-forward as I though. Therefore — this PR might be a bit long but hopefully not too complicated, bare with me :)
The main purpose of this is to bring the recent upstreamed
TargetAddressSpaceAttrand couple it withPointerTypeandGlobalOp. The main challenge is that this collides with the already implemented infrastructure related to AS that revolves around our unified enum approach (That handles numeric and lang AS). My main rationale here is that we let our new attribute to coexist with the already existingcir::AddressSpaceAttr(renamed now). so that we don't break any tests related to offload-type languages.Considering the above what I'm essentially doing is:
TargetAddressSpaceAttrhandle numeric target AS as it is done upstream.AddressSpaceAttrtoClangAdddressSpaceAttr.ClangAddressSpaceAttr(which handles an enum) will handle language/clang specific AS (CUDA, OpenCL, etc..) it will no longer handle target as it used to!PointerTypewill hold:TargetAddressSpaceAttr,ClangAddressSpaceAttror null (default AS). We enforce this via a custom verifier. (I would've preferred to enforce this rule via an AttrConstraint, but apparently that doesn't support types? — At least I couldn't make it work.)GlobalOpswill enforce the rule mentioned above as well via AttrConstraints. This works since we can apply constraints to Ops.target_address_space(n)/clang_address_space(x).mlir::Attribute = {}Important!!
Not in the scope of this PR but my idea is to improve, revamp the later mentioned
ClangAddressSpaceAttrand incorporate most of the feedback we received in: llvm/llvm-project#161028 (comment). (And yes! I haven't forgotten about the {MemorySpaceAttrInterface} to be coupled with the ptr op).